fix(notifications): retain Notification reference so click handler fires#2238
Merged
Conversation
Clicking a desktop notification was supposed to switch the app to the task it was about (via TaskLinkService → deepLink.onOpenTask → useTaskDeepLink → navigateToTask), but the JS click listener was never firing. On macOS the OS still raised the app on click, so it looked like "focus works but navigation doesn't" — which is exactly how this surfaced after PR #2210 started firing notifications for other tasks while the app was already focused. Root cause: ElectronNotifier.notify() created `new Notification(...)` in a local variable and let it fall out of scope after `.show()`. V8 could GC the JS wrapper (and the `click` listener with it) before the user clicked. Fix: hold a reference in an instance Set until the notification is clicked or closed, then release it. No behaviour change beyond keeping the wrapper alive long enough for its events to reach JS. Generated-By: PostHog Code Task-Id: ed71008c-f6e9-4cb8-ad76-006037bcce5b
richardsolomou
force-pushed
the
posthog-code/fix-notification-focus
branch
from
60450be to
c7ce04f
Compare
richardsolomou
changed the title
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/main/platform-adapters/electron-notifier.ts:31-34
**Orphaned reference if `close` never fires**
The `active` Set is only cleaned up on `click` or `close`. On some platforms (notably Windows), a notification that expires via OS timeout may not fire the `close` event — leaving the `Notification` object retained in the Set indefinitely. The same applies if Electron emits the `failed` event (display failure): the notification was already added at line 31 but neither `click` nor `close` will follow. In practice this is low-risk (notifications are rare, objects are small), but it's worth also hooking `once("failed", release)` to cover that path.
Reviews (1): Last reviewed commit: "fix(notifications): retain Notification ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Cover the path where Electron emits `failed` (display failure) instead of `close` / `click`, so the active Set doesn't accumulate references to notifications that were never successfully shown. Same applies to platforms (notably Windows) where OS-timeout dismissal doesn't fire `close` — `failed` is the closest signal we have for that. Generated-By: PostHog Code Task-Id: ed71008c-f6e9-4cb8-ad76-006037bcce5b
charlesvien
approved these changes
May 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After #2210, PostHog Code can fire notifications for a different task while you're already focused on another task in the app. Clicking those notifications was supposed to switch you to the task the notification was about (via
TaskLinkService→deepLink.onOpenTask→useTaskDeepLink→navigateToTask), but the in-app view never changed. On macOS this surfaced as "the app comes to the foreground but stays on the wrong task" — because macOS raises the app on notification click as default OS behaviour, independent of whether the JS click handler runs.Root cause:
ElectronNotifier.notify()(apps/code/src/main/platform-adapters/electron-notifier.ts) creatednew Notification(...)in a local variable and let it fall out of scope right after.show(). With no retained reference, V8 was free to garbage-collect the JS wrapper before the user clicked, taking theclicklistener with it. The OS-level notification still rendered fine, butonClick(wheretaskLinkService.emit(TaskLinkEvent.OpenTask, ...)lives) never fired.Changes
Hold a reference to each shown
Notificationin an instanceSet<Notification>onElectronNotifier, and release it when the notification'sclickorcloseevent fires. This keeps the JS wrapper alive long enough for its events to actually reach JS, with no behavioural change beyond that.How did you test this?
pnpm --filter code typecheck— cleanpnpm lint— cleanelectron-notifier.ts(now retains ref) →notification/service.ts:38-49(onClickemitsOpenTask) →deep-link.ts:27(subscription yields) →useTaskDeepLink.ts:113-123(callshandleOpenTask) →navigationStore.navigateToTask(which usesisSameViewtask-id comparison, so it correctly switches between distinct task-detail views).Publish to changelog?
no
Created with PostHog Code